Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

complete docstrings for ContractFunction and reorder contract classes #2960

Merged
merged 1 commit into from
May 22, 2023

Conversation

pacrob
Copy link
Contributor

@pacrob pacrob commented May 16, 2023

What was wrong?

ContractFunction and AsyncContractFunction were missing params in their docstrings.
Contract files have similarly named classes, but all were in different order.

How was it fixed?

Completed docstrings for above classes and updated associated docs to match.
Ordered related classes similarly across the 3 contract files.

Todo:

Cute Animal Picture

image

@pacrob pacrob force-pushed the update-contract-docstrings branch from e04ca99 to 1d960bc Compare May 16, 2023 23:40
@pacrob pacrob requested a review from kclowes May 17, 2023 17:17
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Had a nit around co-locating ContractEvent/ContractEvents and ContractFunction/ContractFunctions. Feel free to take or leave!

@@ -125,6 +125,132 @@
from .contract import ContractFunction # noqa: F401


class BaseContractEvents:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense for this to be close in location to the BaseContractEvent class, what do you think about that? And similarly, have BaseContractFunctions and BaseContractFunction close as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, I went back and forth on that - ContractFunctions and ContractEvents are the only ones that needed to be on top for dependency ordering, so I went with just floating them up, but no issue with pulling Function and Event up too. Will do.

@pacrob pacrob force-pushed the update-contract-docstrings branch from 6f4c753 to d2091bb Compare May 22, 2023 20:58
@pacrob pacrob merged commit c1a8751 into ethereum:main May 22, 2023
@pacrob pacrob deleted the update-contract-docstrings branch May 22, 2023 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants